Cache activity before opening retirement PRs#120
Conversation
Persist the latest observed merge activity and the first inactive observation between workflow runs. A missing /commits response now starts a confirmation window instead of immediately opening a retirement PR, while cached positive activity can keep active committers from being retired when GitHub returns an empty result spuriously.
MattSturgeon
left a comment
MattSturgeon
left a comment
There was a problem hiding this comment.
Awesome!
I won't pretend to have fully digested the bash impl, so don't be offended if I spot something worth commenting on later.
SGTM overall, though. Nice idea to mitigate GitHub's flakiness!
| name: retire-cache | ||
| path: .cache/retire-cache.csv | ||
| if-no-files-found: error | ||
| overwrite: true |
There was a problem hiding this comment.
Nit: could we do archive: false so the CSV becomes readable in a browser? There should be no need to zip archive it.
This also means we don't need name, as it's ignored when not archiving, path's basename is used instead.
Let's also explicitly set retention-days. The default (and maximum) is 90, but if we schedule this workflow daily we can probably reduce artifact retention to a week or two?
| - name: Restore activity cache | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| run: | | ||
| runId=$(gh run list --workflow=retire.yml --status=success --limit 1 --json databaseId --jq '.[0].databaseId // empty') | ||
| if [[ -n "$runId" ]] && gh run download "$runId" --name retire-cache --dir .cache; then | ||
| echo "Restored activity cache from run $runId" | ||
| else | ||
| echo "No previous activity cache, starting fresh" | ||
| fi |
There was a problem hiding this comment.
This is trivial enough as bash, but if written as a github-script action, we'd gain more explicit handling of error modes (e.g. the API request failing).
Having this small step be github-script could be a small win and set a precedent for later rewriting other scripts too?
Fine as is, though.
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| run: | | ||
| runId=$(gh run list --workflow=retire.yml --status=success --limit 1 --json databaseId --jq '.[0].databaseId // empty') |
There was a problem hiding this comment.
This would benefit from pipefail (etc), which we can enable throughout by setting shell: bash explicitly somewhere:
https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#defaultsrunshell
(Ideally, set the default at the top of the workflow, otherwise set shell: on relevant steps)
| # Matches the merge lines produced by the jq filter below, | ||
| # capturing the merge date and PR number for the cache | ||
| mergeLineRegex='^- `([^`]+)` .*(#[0-9]+)$' |
There was a problem hiding this comment.
This feels a little brittle. Is there a way we can pass the date+number values around in a way that doesn't require string parsing to extract them again?
|
Every one of your comments makes me want to rewrite it in github-script, @MattSturgeon :-) Thanks for taking a look. I'll cogitate on these comments. |
2 participants
I hate those "whoops" issues that #100 creates. Here's one kind of dumb take on fixing it. Sorry for the bash overload. I didn't want to rewrite this in github script first.
What does it do? In short, we persist the latest observed merge activity and the first inactive observation between workflow runs using a GitHub workflow artifact. A missing
/commitsresponse now starts a confirmation window instead of immediately opening a retirement PR. The result is that cached positive activity can keep active committers from being retired when GitHub returns an empty result spuriously.I don't really love the resulting code. I'd sort of like to rewrite the whole thing.
Fixes #100